fix(#5255): read ANSI-C quoting ($'…') in the secret-read guard - #5285

Review of head c444497f861b — changes requested

Summary. The fix does what the issue asks for the five listed commands (all now blocked, reproduced), but the new $-run handling in tokenize runs on every command, not only on $'…' ones, and it opens a bypass that the base blocks: cat $$\<newline> .env. Two further gaps in the same bug class remain allowed. Blocker 1, Major 2, Minor 6, Nit 5.

Classification and gates. #5255 carries bug + confirmed-bug (RULESET.CONTRIB.CLASSIFY.fix=requires confirmed-bug before implementation (legacy 'confirmed' label is back-compat only for duplicate-sweep exemption, not a valid implementation gate)). Fixes #5255, conventional title, not draft, no competing PR. Changeset type Security, pr: 5285, bold lead, (#5255); docs/USER-GUIDE.md updated; CHANGELOG.md untouched. No shipped workflow, agent or command changed (no compact-content baseline or Emitted-Drift-Ack-Growth requirement); no references/ or workflows/ file added.

Acceptance criteria, #5255 (Agent Brief and tests-to-add)

1. deny echo $'x\'y'; cat .env — Met. 2. deny cat $'.e\x6ev' — Met. 3. deny git commit -m $'it\'s' && cat .env — Met. 4. deny x=$(printf $'\''); cat .env — Met. 5. deny cat <<$'EOF' heredoc then cat .env — Met. 6. allow echo $$'x\'; ls — Met. 7. the three scanners change together — Met (shared span-end function; seeded parity properties). 8. $$'x' stays a PID plus plain quote — Met for quote runs; not met for $ runs with a line continuation (Blocker 1). 9. $"…" — Met.

Reproduction (plain node spawning the real hook with a Bash payload on stdin; base = origin/next, head = this PR; exit 2 = block)

Real shells on the review host: /bin/zsh -c "cat $'.env\u'" (and the \x, \U forms) prints the file; /bin/bash (3.2) reads a file literally named .env\u.

Findings

Blocker 1 — a command the base denies is now allowed [verified by running the hook] hooks/gsd-secret-read-guard.js, tokenize dollar branch, the "any other run" block (if (run.dollars > 1) { hasWord = true; buf += str.slice(i, run.end - 1); i = run.end - 1; continue; }): dollarRun consumes a \<newline> that follows the $ run, so run.end - 1 is the newline. The tokenizer resumes there, keeps the backslash in the word and treats the newline as a command separator, so .env becomes a bare command word with no operand. bash joins the lines (cat $$ .env). findSecretRead runs scanCommand(command, depth, true) for commands that contain no $' or $" at all, so this affects ordinary commands, contradicting "nothing a real command denied before can become allowed". The fuzz pass (about 5M mutated commands, base versus head) found only this class (cat$$\<nl> .env.local, the same inside <(…) and "$(…)"). Fix: append the run's $ characters without the continuation, or leave i on the last $; check findParenClose, which uses the same i = run.end - 1 shape; add deny rows and a property over $/continuation mixes.

Major 1 — $'…' inside ${…} still hides the rest of the line [verified] echo ${x:-$'}'}; cat .env and echo ${x//$'}'/y}; cat .env are allowed (bash runs cat .env), while echo "${x:-$'a'}"; cat .env and echo ${x:-$'\''}; cat .env are denied. ${ is taken whole up to the first }, which here is inside the dollar-quoted span. This is the failure the issue describes (a quoted character turns the guard off for the rest of the line) reached through a $'…' span; the source comment records it ("A $'…' inside ${…} is not read") but the changeset and USER-GUIDE text do not. It was allowed on the base too, so it is not a regression, but the change claims the guard now reads $'…' in words, $( ) and heredoc tags. Fix: give the ${ skipper the same span rule, or state the exclusion where users read it and pin it with a test.

Major 2 — zsh-only $'…' escapes still hide a secret name [verified: hook exit codes above; zsh and bash outputs on this host] The decoder models bash and keeps \u, \x, \U, \M- and \C- literally, so cat $'.env\u' is not seen as .env; zsh drops the backslash or truncates at the NUL and reads .env. Allowed on base and head alike. The PR header calls the zsh reading "unmeasured". The Bash tool's shell on macOS is zsh. Fix: decode under zsh rules as well, or deny a $'…' word that holds a bare \u, \x, \U, \M- or \C-.

Minor 1 — no test pins $ runs containing \<newline> [verified] The existing continuation rows only cover runs that reach a quote; this is the root of Blocker 1.

Minor 2 — no test pins the ${…} + $'…' case in either direction [verified] Neither a deny assertion nor a pinned-allow control with a comment that it is an accepted hole.

Minor 3 — the decoder has no committed oracle, and one comment is wrong [verified/judgment] The "18,066 spans against bash 5.2.21" comparison exists only in the PR body; decodeAnsiC has no in-repo oracle test, and the host bash here (3.2.57) already differs ($'.env\c'). The comment says decodeAnsiC models bash, but \x{H…} is a zsh form that bash 5.2 does not define; say which reading each form belongs to.

Minor 4 — the PR goes beyond the brief [judgment, Spec axis] The brief asks for bash's single reading; the PR adds a dual-reading scan (plain and ANSI-C), two new block codes (ansi-c-unterminated, ansi-c-undecodable), invalid-UTF-8 and literal-NUL denial (echo $'\xff', which the issue's "ordinary $'…' word with no secret operand remains allowed" would allow), extra decoder forms (\x{H…}, \c\\, a NUL-truncation second value) and about 400 lines beyond the brief. The PR body states the false-block cost (7 of 423,468 corpus commands) and asks whether the maintainer would prefer the other reading; that question is addressed to the maintainer and is not answered in this review.

Minor 5 — boundary rows are partial [verified] The 1 MiB $-run test covers exactly MAX_COMMAND_LENGTH with no limit-1 or limit+1 row, and nothing exercises the new ANSI-C paths at nesting depth 2, 3 and 4 (RULESET.TESTS.boundary-coverage).

Minor 6 — the property suite is a fixed sample and does not reach the decoder [verified] { seed: 5255, numRuns: 25 } covers a small fixed set of span shapes, and no property (round trip, idempotence, or decode-versus-oracle) targets decodeAnsiC, the parser here (RULESET.TESTS.property-based-testing); the \c, \x{…}, \U and \0 branches each need an assertion that kills them (Stryker 80% threshold, not measurable from the diff).

Nit 1 [verified] tests/gsd-secret-read-guard.test.cjs nulTruncation cases carry literal escape text in place of a dash inside // comments.
Nit 2 [verified] scanXargsPipe has a bare { const hit = wordSecret(w, true); if (hit) return hit; } block inside a loop, and the same one-liner at five call sites.
Nit 3 [judgment] findSecretRead now returns a string, an {code, text} object or null, and the caller discriminates with typeof; a uniform result type removes the sniffing.
Nit 4 [judgment] The ansiC flag is threaded through about ten signatures, and ansiCTagWord re-implements tokenize's word reader, so the word-reading rules live in three places and only the parity property keeps them aligned.
Nit 5 [judgment] nulValue and altText need a comment to say they hold zsh's truncated reading (zshValue/truncatedText), and the file header now carries a roughly 30-line known-gaps essay that belongs in docs.

Considered, not findings: $(echo $(echo $(echo cat .env))) (depth 3, MAX_NESTING_DEPTH) and cat ${x:-.env} are allowed on base and head and are not touched by this diff; no hang or quadratic cost found (about 1 MiB inputs finished in 0.06-0.46 s against 0.05-0.09 s on base); commits carry no Emitted-Drift-Ack-Growth trailer and none is required (the hook ships verbatim, not as an emitted baseline).

Passes run

Not run (Memtrace is unavailable): the Deterministic Review Pass (detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr), preflight_check, risk tiering (find_central_symbols, find_bridge_symbols), the recorded-decision check (recall_decision, governing_rules) and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted); CodeGraph's index is the main checkout, so this PR's new symbols (dollarRun, ansiCQuoteEnd, decodeAnsiC, ansiCTagWord) are absent from it and the hook was reviewed from the diff and by execution.

Laws that apply

Subject: PR #5285 teaches the secret-read guard to read $'…' (ANSI-C) quoting in words, $( ) and heredoc tags, with a dual-reading scan.

• kerckhoffs-principle — fired: a deny-list guard is safe only if it holds when its tokenizer is public, and a public input (cat $$\<newline> .env) now passes where it was blocked (tokenize, dollar-run branch). Action: Blocker 1.
• postels-law — fired: the guard decodes one word several ways (plain, bash, zsh-truncated) and the shell it protects reads a fourth way for \u/\x; the decoder is lenient where it should reject. Action: Major 2, Minor 3.
• leaky-abstractions — fired: the guard re-implements shell quoting, and the zsh/bash difference leaks through decodeAnsiC and ${…} handling. Action: Majors 1 and 2, Nit 4.
• hyrums-law — fired: users' commands with $'\xff' or a literal NUL were allowed and are now denied (Minor 4); stated cost is 7 in 423,468 corpus commands. Action: Minor 4.
• goodharts-law — fired: the parity properties pass on a fixed seed of 25 runs and cannot see the $-run class. Action: Minors 1, 6.
  Considered and cleared: knuths-optimization-principle (no optimization; cost measured and small), kernighans-law (comment mass is a Nit).

CI. At the passes snapshot every row on this head was SUCCESS or SKIPPED with nothing pending, and the branch is BEHIND next by one commit (rebase is a CI condition, not a finding). I am not approving: Blocker 1 and the Majors need a push.

CodeGraph Evidence

Memtrace unavailable: monthly quota exhausted and the server disconnected, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run, and the recorded-decision check was not evaluated.

• codegraph_impact tokenize: the change is confined to hooks/gsd-secret-read-guard.js (runs on every Bash/Read/Grep PreToolUse), its test and one USER-GUIDE line; scanner signatures gained an optional trailing ansiC parameter and internal callers were updated (read from the diff because the branch-new symbols are absent from the main-checkout index).
• codegraph_callers/codegraph_node: file-mode read of the tokenize hunk was refused for this worktree path, so the hook was exercised by executing the extracted head and base copies instead.
• Finding: the blast radius is one guard, but it is the secret-read guard, so a previously denied read becoming allowed (Blocker 1) is high-severity.

Verdict

Changes requested. Blocker 1, Major 2, Minor 6, Nit 5.

Review of head eba8ddee82a9 - substance clean pending maintainer decisions

Prior findings. Re-ran the hook (head vs next): cat $$\<nl> .env, cat $\<nl>$\<nl> .env blocked (Blocker 1 fixed); echo ${x:-$'}'}; cat .env, echo ${x//$'}'/y}; cat .env blocked (Major 1); $'.env\u', \x, \U, \M-, $'.env.\local', \C-@ blocked (Major 2). Minors 1-3, 5, 6 and Nits 1-3, 5 are addressed (rows, oracle fixture, boundary and depth rows, in-process properties, uniform result). Nit 4 declined with a reason I accept.

Open for decision.

• echo $'\xff'; ls is blocked (ansi-c-undecodable); the issue wording would allow it. Keep (9 corpus false blocks) or make bash decoding lenient?
• The zsh reading and oracle could be split out, as the author offered. Keep in one PR?
• claude --agent $'gsd-debugger' bypassing #5214's check predates this PR; file a separate issue?

Nits. Changeset is a ~1.2k-char paragraph; shorten it. The \M-/\C-/\M hook rows are model-derived (no live zsh); say so in a test comment or confirm on a zsh host.

State. Fixed against next by the 8 commits; branch is BEHIND by 15, merges clean; rebase and push. #5309 touches the same hook and tests, so the second to merge will need a rebase. CI on this head: no failures, nothing pending.

Memtrace Evidence

The Memtrace MCP server was unavailable for this review (it failed to connect: memtrace not found in PATH), so no graph-backed check ran. None of the four checks below is a clean result.

• get_impact: not run. Blast radius comes from reading the diff and grepping callers.
• get_symbol_context: not run. Callers and callees of the changed symbols were checked by grep.
• recall_decision: not run. The recorded-decision gate did not execute, so this does not show that no decision governs the change. ADRs and CONTEXT.md predicates were checked by grep instead.
• find_code_review_issues: not run. The deterministic review pass did not run. A manual diff review stood in for it, so the findings above are judgment, not graph facts.

Review — fix(#5255): read ANSI-C quoting ($'…') in the secret-read guard

Head reviewed: 5565152b90ef23d830fd632c60d792d5b4291af4 (fresh gh pr view this turn: mergeable=MERGEABLE, mergeStateStatus=BEHIND, reviewDecision=CHANGES_REQUESTED). Re-review of trek-e's two CHANGES_REQUESTED reviews (c444497f861b, eba8ddee82a9) and the author's three replies.

Summary

Every item from the earlier reviews is fixed on this head (re-verified by running the real hook, see below). The ANSI-C work itself held up under about 100k differential-fuzz commands run through real zsh 5.9 and bash 3.2 (0 bypasses attributable to $'…' decoding). Two live bypasses of the exact bug class remain, both verified against a real shell and both also present on next: (1) in zsh, $$'…' is PID plus an ANSI-C span, not PID plus a plain quote, so echo $$'\''; cat .env reads .env and the hook allows it; (2) in bash, $${ is PID plus {, but the hook swallows the line as an unterminated ${, so echo $${; cat .env reads .env and is allowed. The PR text and USER-GUIDE claim zsh is modelled and that $$'x' is "the PID and a plain quote"; for zsh that is false. Changes requested: 2 Major, 3 Minor, 1 Nit.

Classification & gate compliance

• Issue #5255 carries bug + confirmed-bug (Bug-Fix track). PR has label review: changes requested. Title fix(#5255): … conventional. Not draft. Body has Fixes #5255, follows the Fix template. Branch fix/5255-secret-read-guard-ansi-c-quoting conforms.
• Changeset .changeset/quiet-foxes-quote.md: type Security, pr: 5285, bold lead, trailing (#5255), ~400 chars (earlier nit fixed). CHANGELOG.md untouched. Security type needs no docs/ update; docs/USER-GUIDE.md line updated anyway (see Minor 1 for an inaccurate sentence in it).
• No references/ or workflows/ file added (no inventory drift); no shipped workflow/agent/command changed. install-tree fixtures list the hook by name only, so no fixture co-change.
• Template "Platforms tested": only Linux ticked; macOS unchecked although the zsh behaviour is the macOS Bash tool's shell. zsh checks were run in an Alpine container (per author). See Minor 3.
• Scope: one concern (the secret-read guard). The dual-reading design and the zsh decoder are beyond the issue brief but are disclosed and justified; see decisions below.

Must-Have Acceptance Checklist (issue #5255 Agent Brief / tests-to-add)

Re-run on this head via the real hook (exit 2 = deny):

1. echo $'x\'y'; cat .env denied: PASS (exit 2; allowed on base).
2. cat $'.e\x6ev' denied: PASS (exit 2; allowed on base).
3. git commit -m $'it\'s' && cat .env denied: PASS (author's test row; same shape echo $'a'$'\''; cat .env exit 2).
4. x=$(printf $'\''); cat .env denied: PASS (echo $(echo $'\'') ; cat .env exit 2, depth 2 and 3 also exit 2).
5. cat <<$'EOF' heredoc then cat .env denied: PASS (cat <<$'E\'F'… exit 2).
6. echo $$'x\'; ls stays allowed: PASS on the hook (exit 0), BUT see Major 1: under zsh that command is an unterminated ANSI-C span, so criterion 6 is only true for bash's reading. It conflicts with the zsh model.
7. Unterminated $' or an undecodable escape is not allowed: PASS (cat $'.env exit 2; $'\ud800' / literal NUL covered).
8. All scanners agree on span ends: PASS by construction (shared span-end rule, seeded parity properties).
9. $"…" read as "…" (optional item 5): PASS (cat $".env" exit 2).

Root-cause / functional verdict

Root cause (one sentence): three quote-skipping scanners ended a single-quoted span at the next ' without looking at the preceding backslash, and no $ test paired $ with ', so $'…' was never modelled. The diff addresses that root cause in all three scanners (plus the ${…} skipper) rather than special-casing the five commands.

Open items remaining (not the fixed ones):

• The zsh reading is not complete: it models zsh's $'…' decoder but not zsh's rule that a $ run before ' always opens an ANSI-C span (Major 1).
• The $-run pairing the PR introduces is applied before ' but not before { (Major 2).

Previously reported items, re-verified fixed on this head: Blocker 1 (cat $$\<nl> .env) denied; Major 1 (echo ${x:-$'}'}; cat .env) denied; Major 2 ($'.env\u', \x, \U, \M-, .env.\local, \C-@) all denied; \xff now lenient.

Decisions the author left open: evaluated against the guard's fail-closed posture

Maintainer rulings: item 2 — keep the zsh reading in this PR. Item 3 — the claude --agent $'gsd-debugger' bypass is tracked separately in #5441 and is not required here. Item 1 — recommendation below stands (keep lenient).

1. echo $'\xff'; ls lenient vs strict. Recommendation: KEEP LENIENT (as now implemented in 8d6b40f69). Verified: echo $'\xff'; ls exit 0; cat $'\xff'/.env and echo $'\xff'; cat .env still exit 2; cat $'x\xc3/.env'-style reads remain reads because ASCII bytes keep their place and a non-UTF-8 byte becomes U+FFFD, which no protected name (.env, .env.*, .secrets, matched case-insensitively) can contain. Leniency here changes only the decoded value compared, never whether the rest of the line is scanned, so it does not open a skip. Denying would be a false block with no security value (9 corpus commands). The remaining ansi-c-undecodable denies (literal NUL, code point zsh refuses) are correct fail-closed: there the cut changes which name the shell sees.
2. Split the zsh reading into its own PR. Recommendation: KEEP IN ONE PR. zsh is the Bash tool's shell on macOS; Major 2 of the first review (cat $'.env\u' reading .env under zsh) was real and verified, and splitting would leave a known live bypass on next until a second PR merges. The zsh reading is also where the remaining gap is (Major 1 below); that is a reason to fix it in place, not to defer it. Reviewability cost is mitigated by the committed 1,200-span oracle.
3. claude --agent $'gsd-debugger' bypassing #5214's debugger-cli-spawn check. Recommendation: SEPARATE ISSUE, which now exists as #5441. Different contract (a workflow-integrity check, not secret reads), different file section; one-concern-per-PR applies. Note for the maintainer: CLAUDE.md "No Deferrals" says a defect found anywhere is fixed in the current change; that rule and "One concern per PR" conflict here and the call is the maintainer's.

Blast radius

hooks/gsd-secret-read-guard.js runs on every Read/Grep/Bash PreToolUse. Exports gained { decodeAnsiC, findSecretRead } (new, for in-process tests); no existing consumer imports the hook. Scanner signatures gained a trailing reading parameter; all call sites are inside the file. findSecretRead now returns {code,text}|null (previously string|null) uniformly.
Branch is BEHIND next by 64 commits. git merge-tree reports no conflict; a trial merge (origin/next fb65ab92b0 + PR head, scratch worktree, not committed) merges cleanly. next gained #5478 (scanGitConfig, walkGitGlobals) in the same hook, which calls findSecretRead(script, depth+1). I exercised the merged hook: git -c alias.x=$'!cat .env' x, …$'!cat .e\x6ev', …$'!echo \'a\'; cat .env', git -c $'alias.x=!cat .env' x, git -c diff.external='cat .env' diff all exit 2; git status exit 0. No semantic break found. Rebase is still required (BEHIND) and #5478's tests must run on the rebased tree (CI condition, not a finding).

Co-change completeness

Hook + test + fixture oracle + USER-GUIDE + changeset all present. docs/INVENTORY*, hooks/hooks.json, scripts/build-hooks.js, install-tree fixtures reference the hook by name only and need no change. CONTEXT.md has no new vocabulary. No gap found.

Test quality

• No wall-clock, no source-grep (the test reads a JSON fixture only), fast-check properties present (6 spawn-based parity properties at 25 runs; 2 in-process decoder properties at 2,000 runs; a $/continuation property at 60). Boundary rows at 1 MiB -1/0/+1 and nesting depth 2/3/4 present per author and prior review.
• Fail-first: author reports 75 of 504 hook-level rows fail against next hook and 504 pass on head; plausible (the five issue commands plus decoded-name rows are allowed on base, confirmed by my base/head table). I could not execute the suite (local node --test is blocked; CI is the executor).
• Gap: no row for the two bypasses below, and echo $$'x\'; ls is pinned as allowed (bash-only semantics) without a zsh counter-row.

Memtrace Evidence

• get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
• get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
• recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). No governing-decision check ran; CLAUDE.md/CONTEXT.md/CONTRIBUTING.md were checked manually.
• find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). The deterministic review pass did not run; findings below are manual and executed-evidence based.

Substitute evidence (CodeGraph was unusable for the worktree: "isn't indexed"; the main-checkout index predates the PR's symbols). Instead I executed the real hook from detached worktrees of head (5565152b9), origin/next (f7efbf110a) and a trial merge, against: ~170 hand-built shapes; a decoder-escape differential fuzz (about 40k commands, seeds 1-4, each run in real zsh -f -c and bash --norc -c, flagged when a shell printed the secret file and findSecretRead returned null): 0 bypasses; and a quote/paren/heredoc "token soup" fuzz (200k commands, 11 seeds) that found 60 distinct allowed-but-executed commands, all of which next also allows (0 regressions: none denied on base and allowed on head). Shells on the review host: zsh 5.9, bash 3.2.57 (not 5.2; escapes where 3.2 differs were not used as evidence).

Findings

Major 1 — zsh reads $$'…' as PID + ANSI-C span; the guard reads it as PID + plain quote, so an escaped quote hides the rest of the line [deterministic fact; pre-existing on next, but exactly the bug class of #5255 and contradicts the PR's zsh claims] hooks/gsd-secret-read-guard.js (dollarRun / $$ pairing in tokenize, findParenClose, ansiCTagWord, applied to the zsh reading).
Repro (hook exit codes head=base=0; real zsh 5.9 with a .env in cwd prints it): echo $$'\''; cat .env; also echo $$'\''a<newline>cat .env, echo x$$'\''; cat .env. zsh output of echo $$'\''F is <pid>'F (the span closes); bash errors with "unexpected EOF while looking for matching ''". Other $-prefixed forms ($@', $*', $1', $!', $#', $?', ${x}', $x', $HOME') are NOT affected (checked in zsh: not ANSI-C). Only a $run: the last$of a run before'opens ANSI-C in zsh even when the run is$$. Fix: in the zsh reading, treat a $run followed by'as ending in a$'opener (the run's last$pairs with the quote); keep the pairing-left-to-right rule for the bash and plain readings. Consequence to decide: under the zsh reading</code>echo $$'x'; ls<code> becomes an unterminated span (zsh itself errors on it), so acceptance criterion 6 only holds for bash; either the zsh reading denies it (</code>ansi-c-unterminated<code>, stricter, arguably correct since zsh never runs </code>ls<code>) or criterion 6 is restated as bash-only. Add rows: </code>echo $$'''; cat .env<code>deny,</code>cat $$'''<code>etc., plus a zsh-reading property over$-run lengths 1..4 before '`.

Major 2 — $${ hides the rest of the line in bash [deterministic fact; pre-existing on next; same $-run code this PR rewrote] echo $${; cat .env, echo x$${ ; cat .env, echo $${x; cat .env are all exit 0 on head and base; real bash 3.2 prints the file (bash reads $$ as the PID, then { as a literal; a lone ${ would be a syntax error so the shell never gets there). The tokenizer takes the second $ of $$ with the { as a ${…} opener and skips to a } that never comes, so everything after is data. This is the same desync class: the PR pairs $ runs left to right before ' but not before { (or (; echo $$(; … is not exploitable because bash errors).
Fix: make the ${ test pair the run the same way (only the last $ of an odd-length run opens ${), and add rows for $${, $$$${ , x$${.

Minor 1 — USER-GUIDE and PR body overstate the zsh model [deterministic] docs/USER-GUIDE.md (hook line: "read the way bash and zsh each read it") and PR body ("$$'x' stays the PID and a plain quote"). Both are false for zsh until Major 1 is fixed. Fix with the code, or state the exclusion.

Minor 2 — tests pin echo $$'x\'; ls as allowed without a zsh counter-row [deterministic] The pinned allow encodes bash semantics only; after Major 1 add the zsh-reading expectation explicitly so the divergence is documented, not accidental.

Minor 3 — PR template: macOS not ticked though the zsh behaviour is the macOS tool shell [judgment] The author ran zsh 5.9 in an Alpine container; the Platforms line lists Linux only. Tick or explain.

Nit 1 — "Other fuzz-found allowed commands" are not attributable to this PR [judgment, informational] The token-soup fuzz also found pre-existing allowed shapes involving >()# comment forms, $((") in zsh, and heredoc-in-paren forms (e.g. \<nl>>()#; cat .env); next allows all of them and #5309 owns the findParenClose area. Listed for the maintainer, not blocking this PR on their own.

CI

Head 5565152b90ef23d830fd632c60d792d5b4291af4. Latest-per-name check runs: no check with a conclusion other than success/skipped (42 registrations: 39 SUCCESS, 3 SKIPPED). action_required runs for this head sha: none. Matrix executed. Branch BEHIND next: rebase required before merge.

Verdict

Changes requested. 2 Major (live bypasses in the guard's own bug class, verified on real zsh/bash), 2 Minor, 1 Minor (template), 1 Nit. Substance of the ANSI-C decoding is otherwise clean.

Review — fix(#5255): read ANSI-C quoting ($'…') in the secret-read guard

Head reviewed: e2d3f8ec71f3d5875aa5f41656871c61df5dba48 (fresh gh pr view this turn: mergeable=MERGEABLE, mergeStateStatus=BEHIND, reviewDecision=CHANGES_REQUESTED). Third review. Prior trek-e reviews: c444497f861b, eba8ddee82a9, 5565152b90ef; author replies of 2026-10-15, 10-18 05:15Z and 10-18 10:08Z read in full.

Summary

Every finding from the 5565152b9 review is fixed on this head, re-verified by running the real hook against real bash, zsh and dash: Major 1 (echo $$'\''; cat .env), Major 2 (echo $${; cat .env), Minors 1-3 and the Nit. The author also fixed several $-run shapes beyond the review ("$$(", unquoted heredoc bodies, $${"$$("), and I confirmed each now denies. Differential fuzzing of the head hook against the next hook and three real shells found 0 regressions (nothing next denies is allowed). It did find one live bypass in the PR's own bug class: the "plain" (dash) reading does not pair a $ run, so echo $${$'\''}'; cat .env is allowed and dash reads .env. It is also allowed on next. Changes requested: 1 Major, 1 Minor.

Classification & gate compliance

• Issue #5255: labels bug, confirmed-bug (Bug-Fix track). PR label review: changes requested. Not draft. Title fix(#5255): … conventional. Branch fix/5255-secret-read-guard-ansi-c-quoting conforms. Body has Fixes #5255, Fix template. 19 commits, all conventional fix|test|docs|chore|refactor(#5255).
• Changeset .changeset/quiet-foxes-quote.md: type Security, pr: 5285, bold lead, trailing (#5255). CHANGELOG.md untouched. Security needs no docs/ change; docs/USER-GUIDE.md hook line updated (zsh $$'x' rule is now stated correctly).
• No references/ or workflows/ file added, so no inventory drift. No new vocabulary, so no CONTEXT.md change. Diff is exactly 5 files: hook, test, oracle fixture, USER-GUIDE, changeset.
• Platforms: macOS now ticked with an honest note (macOS conformance tier; zsh measured in Alpine). Linux ticked.
• Scope: one concern. The zsh reading stays in this PR by maintainer ruling (review 3). claude --agent $'gsd-debugger' is tracked in #5441. The "found while testing" gaps are filed as #5523 / #5512 / #5513.
• Branch is BEHIND next. git merge-tree --write-tree refs/prrev/5285 origin/next returns a clean tree (no conflicts) and none of the hook, test or USER-GUIDE paths changed on next since the merge base. A rebase is needed before merge; this is not a content finding.

Must-Have Acceptance Checklist (issue #5255)

Run through the head hook (exit/verdict from findSecretRead, with the matching shell result):

1. echo $'x\'y'; cat .env denied: PASS (allowed on next; bash and zsh read .env).
2. cat $'.e\x6ev' denied: PASS (allowed on next; bash and zsh read .env).
3. git commit -m $'it\'s' && cat .env denied: PASS.
4. x=$(printf $'\''); cat .env denied: PASS.
5. cat <<$'EOF' heredoc then cat .env denied: PASS (author rows plus <<$'E\'F').
6. Unterminated $' / undecodable escape not allowed: PASS (ansi-c-unterminated, ansi-c-undecodable).
7. All scanners agree on span ends: PASS by construction (shared span-end rule, seeded parity properties).
8. Optional item 5, $"…" read as "…": PASS.
9. echo $'\xff'; ls lenient: PASS (verdict null; cat $'x\xc3/.env' still a read).
   Note: the issue's criteria are about $'…'; the bypass in Finding 1 is a $-run problem, not an unmet criterion, but it is the same desync class and is surfaced below.

Root-cause / functional verdict

Root cause (one sentence): the scanners ended a single-quoted span at the next ' regardless of a preceding backslash and never paired $ with ', so $'…' was neither skipped correctly nor decoded; the diff fixes this in tokenize, findParenClose, readHeredocTag and the ${…} skipper through one shared span-end rule and one opensAnsiC decision per reading. It is a root-cause fix, not a special-case of the five issue commands.

Remaining gap in the same class (Finding 1): a command holding $${ or $$( is scanned under three readings, but the third ("plain") reading is the pre-#5255 scan, which opens a ${…} at the second $ of $${, whereas dash pairs $$ as the PID and reads { literally. When the same line also holds a $'\'', bash/zsh readings treat it as ANSI-C, the plain reading treats the ${ as an unterminated brace, and nobody models dash's actual reading.

Blast radius

hooks/gsd-secret-read-guard.js runs on every Read/Grep/Bash PreToolUse. New exports { decodeAnsiC, findSecretRead } (test seam only; no existing consumer imports the hook). Scanner signatures gain a trailing reading parameter; all call sites are inside the file. findSecretRead returns {code,text}|null uniformly. Over 105k differential commands (below) the head hook denied every command next denies (0 regressions); head additionally denies about 2,000 + 3,100 commands that next allowed and a real shell executes as a read. CodeGraph index in this checkout predates the PR symbols, so blast radius came from reading the diff and executing the hook.

Co-change completeness

Hook + test + oracle fixture + USER-GUIDE + changeset all present. hooks/hooks.json, scripts/build-hooks.js, install-tree fixtures name the hook only, so nothing is missing. No gap found.

Test quality

• Regression tests: 697 hook-level and decoder tests per author; author reports 116 of 665 hook-level tests fail against next's hook and all pass on head. Plausible and consistent with my base/head table (every issue command and every new $-run command is allowed on next, denied on head). I could not execute the suite (local node --test is blocked; CI is the executor).
• tests/gsd-secret-read-guard.test.cjs: no wall-clock assertions, no source-grep (the oracle is a JSON fixture loaded as data), fast-check properties present with seeds ($-run property at 60 and 40 runs, decoder properties at 2,000, parity at 25). Boundary rows at 1 MiB -1/0/+1 and nesting depth 2/3/4 are present. Reversion controls for the round's 12 fix sites are reported by the author.
• Gap: no row for the dash-only shape in Finding 1.

Memtrace Evidence

• get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
• get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
• recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). No governing-decision check ran; CLAUDE.md/CONTEXT.md/CONTRIBUTING.md predicates were checked manually.
• find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). The deterministic review pass did not run; findings below are manual and executed-evidence based.

Substitute evidence (CodeGraph had no index covering the PR's new symbols, so none was used): detached worktrees of the PR head (e2d3f8ec71) and origin/next (0dd1845550); the head hook loaded in-process, the next hook loaded from a copy with the stdin listener stubbed and a findSecretRead export appended; each command run through real bash --norc, zsh -f (5.9) and dash in a scratch directory holding a marker .env, flagged BYPASS when a shell printed the marker and the head hook returned null. Runs: 9 hand-built shapes and 11 targeted shapes (all review and author claims), 45,000 token-soup commands (3 seeds, heredoc, backtick, <(), (( included) and 80,000 $-run / quote / brace commands (4 seeds, no heredoc or backtick). Result: 0 regressions against next. 56 + 27 allowed-but-executed commands, all also allowed on next: the first batch are the #5523 / #5513 families (heredoc tags holding ${ or a backtick, <()/>() comment forms, zsh $(($"))); the second batch are all dash-only and all contain a $'\'' next to $${, $$( or ${x:- (Finding 1). Shell versions on this host: zsh 5.9, bash 3.2.57, dash (macOS /bin/dash).

Findings

Major 1 — dash pairs $$ before {, but no reading models that together with $'…' as a plain quote, so echo $${$'\''}'; cat .env is allowed and dash reads .env [deterministic fact; also allowed on next; same $-run desync class this PR rewrote] hooks/gsd-secret-read-guard.js (the plain reading's ${ test in tokenize, findParenClose and the ${…} skipper for a command that holds $${ or $$().
Repro (head verdict null, next verdict null; dash prints <pid>{$\}# then the marker):

echo $${$'\''}'; cat .env
echo $${x:-$'\''' ; cat .env #
echo $'}'$${$'\''' ; cat .env #
echo "$$("x$'\''${$'<newline>cat .env

dash: $$ is the PID, { is literal, $ is literal and '\'' / '}' are plain quotes, so cat .env runs. bash fails the line (unterminated quote), so the bash reading is right to see nothing; zsh opens ${…} at the run, so it too sees no read. The plain reading opens ${ at the second $ of $${, finds the $'\'' plain quotes close, and the } sits inside the next plain quote, so the brace never closes and the rest of the line is data. The PR's own text says the plain reading "pairs neither" run and is the pre-#5255 scan; that is what leaves this gap. 27 of 80,000 commands in a quote/brace-only generator hit it; all 27 are dash-only.
Fix: make the third reading the dash reading: pair a $ run left to right before { and ( as bash's reading already does, with plain quotes (no $', no $"), and keep it scanned whenever a command holds $${ or $$( (the existing trigger). Add the four rows above as deny rows named dash reading, plus a property over $ run length 1..6 before { followed by $'\''.

Minor 1 — branch is BEHIND next [deterministic fact] mergeStateStatus=BEHIND. git merge-tree --write-tree against origin/next (0dd1845550) is clean and none of the PR's paths changed upstream since the merge base. Run git fetch origin && git rebase origin/next and push. Note #5309 lands after this PR by the author's stated order; no content change needed here.

CI

Head e2d3f8ec71f3d5875aa5f41656871c61df5dba48. Latest-per-name collapse of the check runs: no check with a conclusion other than success or skipped (48 registrations per the status rollup: 45 SUCCESS, 3 SKIPPED). action_required workflow runs for this head sha: none. The matrix executed. No pending run IDs to report.

Verdict

Changes requested. 1 Major (live dash-only bypass of the guard's own bug class, verified in a real shell, present on next too), 1 Minor (rebase). All prior findings and the issue's acceptance criteria are satisfied on this head; substance is otherwise clean.

Re-review — fix(#5255): read ANSI-C quoting ($'…') in the secret-read guard

Head reviewed: 7d30d2cf4357a6267400ab70677b2322e7057a84 (mergeable/mergeStateStatus reported UNKNOWN while GitHub recomputed; previously BEHIND). Prior review was on e2d3f8ec71; the author replied 2026-10-20T15:39Z with three commits (16227f7e4, 3fd90e33b, 7d30d2cf4) plus a rebase. CI at this head: 31 distinct check names, latest-per-name all SUCCESS or SKIPPED; no failing check and no action_required runs.

Method: the head and next hooks were loaded in-process and ~100k generated commands (five token mixes: line continuation, $', $", $$ runs, $((, backticks, heredocs) were run through real bash 3.2, dash and zsh 5.9 in a sandbox holding a marker .env. No node --test was run.

Prior findings

Summary

The prior Major is fixed and the differential found no regression except one live fail-open. Changes requested.

Findings

Major

1. Fail-open on RangeError lowers the deny threshold below next: a command next denies is now allowed. hooks/gsd-secret-read-guard.js:2011 (the outer catch { crash(ON_CRASH…) }, ON_CRASH=ALLOW at :146). On Node 26.11.1 with findSecretRead('cat .env; ' + '"$('.repeat(n)), the last non-throwing length is 15,232 characters on head and 20,827 on next. At 5,400 repetitions (16,009 characters) next returns .env (deny) and head throws RangeError, which the hook turns into allow. The author acknowledges the regression and asks whether to fix; it is a defect of this diff. In main() (and the plain-reading call in findSecretRead) treat RangeError as a deny (command-too-complex), or make the nested-$( scanner iterative, and add a test at a depth that overflows on the CI Node.

Minor
2. Quadratic cost on $$( runs: 1 MiB of $$( takes 7.3 s per Bash call (3 s for $$$() against 11 ms on next (the author measures 9.6 s). The hook runs on every Bash call, so a pathological command stalls the session, and the cost is stated only in a PR comment, not in the file's "Documented gaps" header. Bound the run scan (cap the run length, deny command-too-complex above it) or make the pairing linear, and document it.
3. Still allowed in every reading (each read in a real shell, and also allowed on next): cat ${x:-$'.env'}, cat ${x-$'.e\x6ev'} and cat ${x:+a}$'.env' (bash and zsh print the file); heredoc tags holding ${ (<<${;cat .env, <<$'E'${; cat .env #, <<}; cat .env #, $(E<<\n)\ncat .env); and zsh $(($"))-style arithmetic (x$(($"));cat .env, $((")); cat .env #) and ; cat .env${}. The changeset and USER-GUIDE promise "judged by the value bash and zsh each decode". Close these in this PR (deferral is not permitted) or list the exact residual shapes in the hook header's "Documented gaps" so the promise is not broader than the behavior.
4. Branch needs a rebase: git fetch origin next && git rebase origin/next; the new sha needs a fresh verification pass.

Nit
5. .changeset/quiet-foxes-quote.md does not mention the dash reading added this round and states $$-then-{ only as "the PID, then a literal {".

Clean checks

The head-versus-next fuzz (~54k commands) found no regression other than finding 1. Tests have no wall-clock assertions, no source reads of the hook, seeded fast-check properties (25-2000 runs), 1 MiB-1/1 MiB/1 MiB+1 boundary rows and nesting rows at depths 2/3/4. Gates (carried): Fixes #5255, confirmed-bug, changeset Security pr: 5285.

Memtrace Evidence

The Memtrace monthly quota is exhausted for this run, so none of the graph tools below were executed. These are not-run, not passed; findings come from the isolated manual review only.

• get_impact: not run (quota exhausted). Blast radius and risk tier were not computed from the graph.
• get_symbol_context: not run (quota exhausted).
• recall_decision: not run (quota exhausted). The recorded-decision gate did not execute.
• find_code_review_issues: not run (quota exhausted). The deterministic review pass did not execute.

Verdict

Changes requested. Every finding above must be resolved before approval; a rebase clears base drift only and retires none of them. This review does not release or re-run CI.